Skip to content

Add -z,origin linker flag on OpenBSD. #5196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

3405691582
Copy link
Member

This is required for $ORIGIN rpath processing here (without having to fiddle with workarounds like LD_LIBRARY_PATH).

@@ -168,6 +168,10 @@ set_target_properties(Foundation PROPERTIES
INSTALL_RPATH "$ORIGIN"
INSTALL_REMOVE_ENVIRONMENT_RPATH ON)

if(CMAKE_SYSTEM_NAME STREQUAL "OpenBSD")
target_link_options(Foundation PRIVATE "LINKER:-z,origin")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the targets in the Foundation project (both those here as well as those in swift-foundation) use ORIGIN-based rpath values. Is there something special about the Foundation library, or should this be provided for all of these targets?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-z origin is a flag that is useful in the odd case that the reference to $ORIGIN does not occur in DT_NEEDED or DT_RUNPATH (DT_RPATH also applies, but that is considered deprecated) and is expected to possibly occur at runtime through libdl (i.e. dlopen). In such cases the computation may be elided as the value is unneeded and could cause a failure otherwise due to the missing value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On OpenBSD, -z origin is also required to properly resolve $ORIGIN.

I'm fairly sure this will be required for the other targets, and only the main Foundation library has posed a problem while trying to get the toolchain stood up end to end, so I've only added it for Foundation so far.

It probably is the case I should add them for the other targets, but I need to double-check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, this is Open, not Free. Open and DragonFly both need -z origin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I forgot to close the loop here and mention that I added the flag for the other targets.

Copy link
Member Author

@3405691582 3405691582 Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure this is working as intended. This is only evaluated once before any targets are defined, right?

For now, I've just made it unconditional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be evaluated once. It could be that my regex is incorrect. But, CMAKE_INSTALL_RPATH is user supplied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that I am guessing that cmake is evaluating CMAKE_INSTALL_RPATH at the time this expression is evaluated -- before the INSTALL_RPATH target property is evaluated on the Foundation target. If CMAKE_INSTALL_RPATH doesn't already mention $ORIGIN, this won't add the linker flag globally.

If we were going to apply the linker flag to all targets and only all targets at the top level, we'd need to iterate all targets and get the INSTALL_RPATH target property and add the linker flag for only those targets whose INSTALL_RPATH property mentions $ORIGIN -- but that's a whole lot of messing around.

Hence, it might just be simpler to just add the flag unconditionally for now and be done with it. It's not ideal but it'll work, and the scope of the diff will be smaller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see your confusion now. CMAKE_INSTALL_RPATH is not a target specific property. It is a default value property. The user specifies that and the value is used to initialise the property on all targets. That is:

cmake -B out -G Ninja -S . -D CMAKE_INSTALL_RPATH="$ORIGIN" -D CMAKE_SYSTEM_NAME=OpenBSD -D CMAKE_SYSTEM_PROCESSOR=AMD64

Copy link
Member Author

@3405691582 3405691582 Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point I was missing was forgetting to bring the INSTALL_RPATH property on each target up to the top level and actually set that default. D'oh! I've done that now. plutil still needs a target-specific INSTALL_RPATH, but it doesn't hurt to just override the default for that specific target.

I think this should all make sense now.

@3405691582
Copy link
Member Author

@swift-ci please test.

Copy link
Member

@finagolfin finagolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, especially since confined to OpenBSD.

@3405691582 3405691582 force-pushed the origin branch 3 times, most recently from 97a150b to 800f61d Compare April 14, 2025 22:17
@3405691582
Copy link
Member Author

@swift-ci please test.

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@3405691582 3405691582 force-pushed the origin branch 2 times, most recently from 5b19c08 to 6cc3ff1 Compare April 15, 2025 21:18
This is required for $ORIGIN rpath processing here (without having to
fiddle with workarounds like LD_LIBRARY_PATH).
@3405691582
Copy link
Member Author

@swift-ci please test.

@3405691582
Copy link
Member Author

@swift-ci please test Windows platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants